Skip to content

XWIKI-23830: Only display features in the editor when the backend supports it#5337

Open
ClementEXWiki wants to merge 8 commits intoxwiki:masterfrom
ClementEXWiki:feat/syntaxes-config
Open

XWIKI-23830: Only display features in the editor when the backend supports it#5337
ClementEXWiki wants to merge 8 commits intoxwiki:masterfrom
ClementEXWiki:feat/syntaxes-config

Conversation

@ClementEXWiki
Copy link
Copy Markdown
Contributor

@ClementEXWiki ClementEXWiki commented Mar 24, 2026

Jira URL

https://jira.xwiki.org/browse/XWIKI-23830

Changes

Description

  • Disable features unsupported by the backend in the editor

Clarifications

Screenshots & Video

N/A

Executed Tests

  • Added tests for ensuring whitelisted and non-whitelisted features are taken into account

Expected merging strategy

  • Prefers squash: Yes
  • Backport on branches:
    *

/**
* List of supported syntaxes, along with their configuration
*
* @since 18.1.0RC1
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrong since

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

designSystem: string,
offline: boolean,
editor: string,
syntaxes: SyntaxConfig[],
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As already mentioned, api should only be touched to remove things out of it. We never want to make it grow.
Why do you need to add new properties here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would a specific backend register its supported syntaxes?

Copy link
Copy Markdown
Contributor

@manuelleduc manuelleduc Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By registering a component with a specific role and name, then a lookup by the role and name.
Where the name is the registered syntax configuration.

Copy link
Copy Markdown
Contributor

@manuelleduc manuelleduc Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the package path be [...]/core/syntaxes/syntaxes-config?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would seem weird that the platform name of the package doesn't appear in the path

/**
* List of supported syntaxes, along with their configuration
*
* @since 18.2.0RC1
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

18.2.0RC1 was released a few hours ago ;)

* 02110-1301 USA, or see the FSF site: http://www.fsf.org.
*/

import { generateConfig } from "../../../vite.config";
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can now use @xwiki/platform-tool-viteconfig (since 18.2.0RC1) instead of relying on relative paths.
The same is also true for tsconfig that can now be imported from @xwiki/platform-tool-tsconfig

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't aware of that, thanks :)
I updated the code.

expect(overrideFnCalledWithUrl).toBe(SMALL_IMG_DATA_URL);
});

test("Whitelisted syntax features should be available", async ({ mount }) => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYT of using allow/deny lists instead of black/white lists?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems clearer to me to use whitelist and blacklist as they convey the meaning in a clearer way IMO. Everything not in a whitelist is denied, everything not in a blacklist is allowed.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well to be clear the words black and white are badly connotated and allow/deny have clear and similar meanings. Therefore, we tend to use allow/deny in the code base. So it's better to avoid black/white to stay uniform with the rest of the code.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That connotation is a good point indeed. I updated the PR.

query,
);

// NOTE: there is no "clean" way to filter these elements as of now, so we rely on their hardcoded title instead
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it working with a local different than english?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Fixed it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants